Build wasmtime with wasmtime-c-api-impl crate#501
Conversation
leonm1
force-pushed
the
build/wasmtime
branch
3 times, most recently
from
6bcf94e to
144d005
Compare
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
A few high-level comments:
- Why? Looking at the changes, the benefits don't seem very obvious.
- Always using prefixed variant seems fine (although, I think there was a reason why it wasn't the default - perhaps something with Envoy build?), but is that something that's at all relevant with the move to Wasmtime C++ API in #503?
- Using
wasmtime-c-api-impland always using prefixed variant are pretty separate changes, so might be good to split them off (but if they are too intertwined and would result in a throwaway work, then we can keep this as-is).
leonm1
left a comment
leonm1
left a comment
There was a problem hiding this comment.
Why? Looking at the changes, the benefits don't seem very obvious.
Two things:
- Updating wasmtime resulted in our rust_static_library in wasmtime.BUILD failing to build due to missing dependencies. With this current approach, the dependency tree is resolved by Cargo and we simply wrap it in rust_static_library. Updates which pull in new dependencies are trivial after this change and (in theory) will never require hunting down missing dependencies.
- For prefixing itself, using parts of the wasmtime_c_api (as opposed to the raw wasm_c_api in wasm.h / required for limits) involves a whole tree of c header files in the c-api crate. The approach we used previously, prefixing individual files via genrule, doesn't work with globs for all the required files (as bazel requires declaring all genrule outputs), so the toil and complexity of the old build would be increased significantly to support limits.
Always using prefixed variant seems fine (although, I think there was a reason why it wasn't the default - perhaps something with Envoy build?), but is that something that's at all relevant with the move to Wasmtime C++ API in #503?
Envoy build itself ran the multi runtime tests, so I don't think it required the non-prefixed version.
Using wasmtime-c-api-impl and always using prefixed variant are pretty separate changes, so might be good to split them off (but if they are too intertwined and would result in a throwaway work, then we can keep this as-is).
Ack. I'm considering this a "change how we build wasmtime" PR, which makes sense semantically. While they are separable, due to higher priority tasks on my todo list, I simply don't have a couple hours to spare to split this.
I don't believe that was ever the case, and I cannot find a code that would support that. Do you have a link? If not, could you open a draft PR against Envoy (you'll need to do that anyway at some point) and verify that it builds fine with Wasmtime - it can be either this PR or off the last PR in the stacked series. |
envoyproxy/envoy#43920, made some additional minor changes to fix the build. |
PiotrSikora
left a comment
PiotrSikora
left a comment
There was a problem hiding this comment.
LGTM (assuming that all Envoy tests pass), but could you shorten the commit subject? Perhaps drop the "crates_universe-provided" part?
Also, after taking another look at it, I'd probably remove the prefixed version altogether, since I don't think anybody is shipping with multiple engines (but maybe I'm wrong?).
leonm1
changed the title
|
Circling back on the prefixed variant and use of multiengine in real products - it looks that ATS supports multiple Wasm engines, but it builds them using standard tools, and not our Bazel build process, so it doesn't get the prefixed version anyway (and it has a warning about conflict between Wasmtime & WAMR due to the same exported symbols). |
|
The commits still exist, so I am okay if we drop the prefixed variant in this PR and can discuss adding it back in conjunction with supporting it in Envoy. |
The only value from prefixed variant (assuming nobody is using it in production) is that we could do: bazelisk test --define engine=multiengine //test/...and it would run tests across all the Wasm engines, instead of requiring per-engine invocation. |
|
I currently use this regularly: However, in the interest of getting the wasmtime update merged, I'm happy to drop the prefixed variant for now, and reintroduce it "properly" later. |
Oh, no... it's not a blocker or a requirement (I already approved this PR) - I was just wondering if it provides any value, since there is clearly some minimal maintenance overhead. |
leonm1
force-pushed
the
build/wasmtime
branch
2 times, most recently
from
fc45c81 to
3433eff
Compare
Required for proxy-wasm#501 Signed-off-by: Matt Leon <mattleon@google.com>
Required for proxy-wasm#501 Signed-off-by: Matt Leon <mattleon@google.com>
Required for #501 Signed-off-by: Matt Leon <mattleon@google.com>
Always uses prefixed wasmtime-c-api-impl, otherwise the prefixed implementation bitrots. Prefixes the headers in the repo rule to allow for globbing, which is not possible in a genrule. The crates_vendor-provided wasmtime-c-api-impl provided build allows us to upgrade wasmtime solely by changing the version number in the Cargo.toml file (and the corresponding repo in bazel/repositories.bzl for the C headers). Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
Signed-off-by: Matt Leon <mattleon@google.com>
proxy-wasm#523 demonstrated that source rewriting via patch_args is expensive. Signed-off-by: Matt Leon <mattleon@google.com>
2 participants
Always uses prefixed wasmtime-c-api-impl, otherwise the prefixed implementation bitrots. Prefixes the headers in the repo rule to allow for globbing, which is not possible in a genrule.
The crates_vendor-provided wasmtime-c-api-impl provided build allows us to upgrade wasmtime solely by changing the version number in the repositories.bzl and Cargo.toml files.
Build off of #496